I put this back to draft because I'd like to change one thing. When rendering forms for polymorphic data types, especially in LiveView, a common use case would be a form in which you can dynamically add more entries and select the concrete type for each of those entries. This is not possible with the current polymorphic_embed_inputs_for/4 function, since it requires you to pass the type identifier.

For the new polymorphic_embed_inputs_for/3 function, I would prefer to pass the schema instead, so that the function can determine the type automatically from the data or the parameters. Therefore, I'd like to add a get_polymorphic_type/3 function to the Html module, which takes the form, schema and field as parameters.

I made the described updates.

• PolymorphicEmbed.HTML.Form.get_polymorphic_type(form, schema, field)
• PolymorphicEmbed.HTML.Form.polymorphic_embed_inputs_for(form, field)

Please have a look.

Great work @woylie ! Sorry but I've been away from Elixir for some time, it's been a looong looong time 😰

You confused me with something now, you're not passing the type at all (as opposed to the existing polymorphic_embed_inputs_for/4).

Do we even need the type at all even for the existing function? Does it have any use for any case? (again, been a long time, might miss something obvious 😂).

Great work @woylie ! Sorry but I've been away from Elixir for some time, it's been a looong looong time 😰

You confused me with something now, you're not passing the type at all (as opposed to the existing polymorphic_embed_inputs_for/4).

Do we even need the type at all even for the existing function? Does it have any use for any case? (again, been a long time, might miss something obvious 😂).

Hi @mathieuprog! Before this PR, the library didn't have a function for determining the type automatically, and that's why it has to be passed to the old polymorphic_embed_inputs_for/4. We can change the existing function to use the new get_polymorphic_type/3 function, of course, and remove the type argument. I just didn't want to introduce any breaking changes in this PR.

Could you add that change? I will just release a major version as there is a breaking change.

Sure!

When I just remove the type parameter from polymorphic_embed_inputs_for/4, all tests pass except for form with polymorphic embed to nil. The channel value is nil in that test, which of course means there is no data from which we can derive the type. The previous implementation would just return empty inputs for the type that was passed to the function.

For the new polymorphic_embed_inputs_for/3 function, we can change it to not render any inputs if the polymorphic field is nil. If you want to initialize the form with empty inputs of a certain type, you can still initialize the changeset that way.

We can also keep the /4 version of the function additionally to the new /3 version, but I don't find it very useful in the first place, to be honest.

If you want to initialize the form with empty inputs of a certain type, you can still initialize the changeset that way.

Which way?

Okay got it. The /4 returns empty inputs. The /3 doesn't render anything, right? That's quite big change.

Let's keep the /4 for now, but add a /3 version.

I realize my own app depends on these empty fields lol. So keep the /4, add the /3. And I'll see with my own app how I should refactor it to only use /3, or we just keep /4 if the code is easier for getting these empty fields.

If you want to initialize the form with empty inputs of a certain type, you can still initialize the changeset that way.

Which way?

You can do something like

changeset = reminder_changeset(%Reminder{},  %{"channel" => %{"__type__" => "sms"}})

to initialize the form with empty fields.

Okay got it. The /4 returns empty inputs. The /3 doesn't render anything, right? That's quite big change. But I don't think we ever really need that type statically?

It's a breaking change, but it's easy to work around, and it makes more sense to me. The static type isn't very useful in a polymorphic situation, unless you want to render a certain type by default if no value is given, but I find this behaviour a bit quirky and would rather initialize the changeset as described to do that.

I realize my own app depends on these empty fields lol. So keep the /4, add the /3. And I'll see with my own app how I should refactor it to only use /3, or we just keep /4 if the code is easier for getting these empty fields.

I see. We're initializing the changeset as described in our application to get those empty fields (did I mention you can do that? 😅 ).

I'll update the PR to include both variants, and you can decide whether you want to deprecate /4 at some point.

Yep just keep the /4 for now and I'll try your suggestion in my app! I'll merge your PR in master as soon as it is ready. Can you comment here when it's ready for merge? Thanks!

Ok, I updated the PR. I replaced the polymorphic? variable in the tests with a generator variable to switch between the three functions in safe_inputs_for.

You loop through generators:

@generators [:not_polymorphic, :polymorphic, :polymorphic_with_type]

defp get_module(name, generator) when generator in [:polymorphic, :polymorphic_with_type],
  do: Module.concat([PolymorphicEmbed, name])

defp get_module(name, :not_polymorphic),
  do: Module.concat([PolymorphicEmbed.Regular, name])

for generator <- @generators do
  # ...
end

Now most of the tests will be run 3 times, including 2 times for the exact same code?

It runs the same tests on both of the polymorphic_embed_inputs_for functions, one time with the given type and one time with the automatically determined type. I think that's fine, and the test suite doesn't take long anyway? If you decide to remove the older function in the future, you can just remove it from the generator list.

1.10.0 has been released but major update 2.0.0 coming tomorrow :D

Thanks!

Btw you change

type_name |> to_string() to
to_string(type_name)
removing the pipe.

Keyword.get(type_opts, :identify_by_fields, []) |> Enum.map(&to_string/1) to
type_opts |> Keyword.get(:identify_by_fields, []) |> Enum.map(&to_string/1)
adding the pipe.

Do you have any rule you made for yourself?

Those are two rules that Credo has checks for:

• single pipe (https://github.com/rrrene/credo/blob/master/lib/credo/check/readability/single_pipe.ex#L8)
• pipe chain start (https://github.com/rrrene/credo/blob/master/lib/credo/check/refactor/pipe_chain_start.ex#L10)

I find both reasonable and usually have them enabled in my projects.

https://elixirforum.com/t/polymorphic-embeds-in-ecto/31951/9

Hello @woylie
I can't make this work with LiveView 0.18
Could you have a look please? I'm not experienced enough with LV.

@mathieuprog I'll check tomorrow.

Ty! The error I'm having when upgrading to 0.18 is the following:

== Compilation error in file test/polymorphic_embed_test.exs ==
** (CompileError) test/polymorphic_embed_test.exs:2158: undefined function sigil_H/2
(elixir 1.12.3) src/elixir_locals.erl:114: anonymous fn/3 in :elixir_locals.ensure_no_undefined_local/3
(stdlib 3.15) erl_eval.erl:685: :erl_eval.do_apply/6
(elixir 1.12.3) lib/kernel/parallel_compiler.ex:428: Kernel.ParallelCompiler.require_file/2
(elixir 1.12.3) lib/kernel/parallel_compiler.ex:321: anonymous fn/4 in Kernel.ParallelCompiler.spawn_workers/7

This is really a mystery for me.

Ah, that's easy, a bunch of functions were moved from Phoenix.LiveView.Helpers to Phoenix.Component, including that sigil. If you update the import accordingly, it should work.

In the docs the example says to use:

use Phoenix.Component

https://hexdocs.pm/phoenix_live_view/Phoenix.Component.html#sigil_H/2

However an import works...

__using__ does a little bit more. You need to use the module in order to use declarative assign. But here, we only need to import the sigil, so import should be fine.

@woylie I only noticed there were some inconsistencies in that PR following this open issue:
#63

I had to remove these clauses:

%{"__type__" => type} ->
  maybe_to_existing_atom(type)

%{__type__: type} ->
  maybe_to_existing_atom(type)

7939f1c

The issue is still open and waiting for feedback to add the right test.

All the tests are running without the clauses above, so the code seemed useless, until we find the right test case.

Just FYI

I'll have to have a look at the application where we first added that, I don't remember why this was added.

Ok note that there is a custom type field for :channel:
https://github.com/mathieuprog/polymorphic_embed/blob/v3.0.5/test/support/models/polymorphic/reminder.ex#L20
And that was the source of the confusion where the tests didn't take that into account.
You can see in the commit above that I replaced __type__ by the custom field name, and the tests run successfully even when removing the clauses above.

After upgrading to 3.0.5, get_polymorphic_type/3 returns nil in our application, which then breaks the form. We don't set the type_field option. I guess we'll first need to check whether type_field is set, and if not, fall back to the stuff you removed? I can try to write tests for that, but I'm not sure whether I can get to that today.

I'm sorry, on the other hand, nice that we have a failing case. I suspected it hence I reach out to you immediately so that we can fix it.

I would say, just add the failing test, and I'll code the fix. I'm not an active user of LiveView, I don't grasp or fully remember the case we're talking about, so it would be nice to have an example that fails in the test.

Most likely we should use the already existing function get_polymorphic_module(schema, field, type_or_data) where type_or_data can be that map. This gives the right module and then we can call get_polymorphic_type(schema, field, module_or_struct). Seems easy once we have the failing test.